-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RHCLOUD-35554] - Add replica control on deployments #199
Conversation
/retest |
/retest |
The security scan is popping off, but I think that should be addressed in a separate MR. |
@@ -115,6 +115,7 @@ type FrontendSpec struct { | |||
ServiceTiles []*ServiceTile `json:"serviceTiles,omitempty" yaml:"serviceTiles,omitempty"` | |||
// Data for the available widgets for the resource | |||
WidgetRegistry []*WidgetEntry `json:"widgetRegistry,omitempty" yaml:"widgetRegistry,omitempty"` | |||
Replicas *int32 `json:"replicas,omitempty" yaml:"replicas,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity. Does this need to be a pointer? I know the check would have to change bellow but I am curious why we prefer pointer here over plain int value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I could be wrong so it isn't a hill I would die on, but as I understood it if the type in the spec is int, then if the property is omitted from the resource then the value will be implicitly set to 0, which would then make my comparison to 0. 0 is a valid value for replicas, and can even be useful in some situations for scaling down and then up again. A corner case, but it has happened. More importantly, it would mean that my logic would be"if the value is 0, set the value to 1" which violates the principle of least surprise. So, I thought this was more correct. That said if any part of my understanding of how all of this hangs together is wrong I'm happy to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Ok, I was not aware that 0 is a valid replica value. I get it now.
I can confirm that the security issue is not related to your changes. We had similar output in previous PRs |
Add the ability to set the replicas for deployments created by the operator. I made this a user controlled value in the frontend spec because I didn't want to unilaterally change the replica count for every frontend, everywhere in our environment. We have some frontends, like chrome our entrypoint, that we definitely want multiple replicas for, but for others maybe we don't.